Hard latency restriction for the PulseAudio backend.#170
Hard latency restriction for the PulseAudio backend.#170szlop wants to merge 6 commits intobastibe:masterfrom
Conversation
…player, record and recorder. The arguments takes an integeger value and restricts the latency of the PulseAudio buffering to the given number of sample frames. If maxlatency is set, buffer underflows or overflows will occur, when the processing cannot keep up.
…member functions of the PulseAudio back end. If set to True, debug information will be printed to the terminal, whenever a buffer underflow or overflow occurs during playback.
Added short explanation of maxlatency and report_under_overflow parameters.
bastibe
left a comment
bastibe
left a comment
There was a problem hiding this comment.
Very nice! I like your additions.
Two small requests:
- reformat the docstrings to the common line length
- use a warning instead of print, same as on the Windows side
Speaking of the Windows side, we don't have a report_under_overflow over there, but simply always report errors when they occur. I think we should do a similar thing here, as I can't think of a situation where I wouldn't want to know about an under/overflow. (And if we're using warnings, the user can still opt to install a warning-filter)
soundcard/pulseaudio.py
Outdated
| might be necessary for short block lengths or high | ||
| sample rates or optimal performance. Default is ``False``. | ||
| maxlatency : int | ||
| Linux only: restrict latency to maxlatency sample frames. If set, buffer underflows or overflows will occur when |
There was a problem hiding this comment.
please restrict line length of docstrings.
There was a problem hiding this comment.
I guess docstrings should be limited to 72 characters, right?
Pycharm is complaining about some format errors in the pulseaudio.py file, e.g., PEP 8: E302 expected 2 blank lines, found 1 above the function headers. Is there a reason for the existing format or could we just hit "reformat" and be done with this? :)
There was a problem hiding this comment.
So long as it's a few changes, I'm fine with that. Just make sure it's in a separate commit so it's easy to revert if necessary.
I guess docstrings should be limited to 72 characters, right?
Yes. Don't worry if it's a few characters too much, though, I don't follow these rules religiously.
There was a problem hiding this comment.
I gave some love to the docstrings and the overall format. It's just a proposal, I won't be insulted, if you reject the changes. :)
There was a problem hiding this comment.
Looks good overall. There are a few details I'd do differently, but not enough to warrant a redaction. Thank you!
| if self._report_under_overflow: | ||
| @_ffi.callback("pa_stream_notify_cb_t") | ||
| def overflow_callback(stream, userdata): | ||
| print('Overflow detected.') |
There was a problem hiding this comment.
On the Windows side, we used a custom SoundcardRuntimeWarning instead of print: https://github.com/bastibe/SoundCard/blob/master/soundcard/mediafoundation.py#L772
There was a problem hiding this comment.
Thanks for the hint. I was wondering how to report the event properly.
…s proposed by PEP8.
- Replaced all occurrences of pulseaudio in comments by upper case Pulseaudio. - Replaced all occurrences of numpy in comments by upper case Numpy.
2 participants
The changes add two optional arguments to the player and recorder functions:
is translated to the PulseAudiobufattr.maxlength`` parameter, which sets a limit for the internal audio buffer and such restricts latency.report_under_overflowis an optional boolean debug argument, which enables the reporting of buffer underflows and overflows to the terminal.The changes are meant for use-cases, which prioritize a defined latency over interruption-free playback/recording.